Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fake pagination for sled_list_uninitialized #4720

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

andrewjstone
Copy link
Contributor

No description provided.

Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very close; thanks for doing it

Comment on lines 4662 to 4665
_query_params: Query<PaginatedById>,
) -> Result<HttpResponseOk<ResultsPage<UninitializedSled>>, HttpError> {
let apictx = rqctx.context();
let handler = async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I don't think we want to offer a sort_by query parameter that we ignore
  • I'm fine ignoring the limit parameter, but perhaps we could use it below
  • I think we want to produce an error if a caller specifies a page token
Suggested change
_query_params: Query<PaginatedById>,
) -> Result<HttpResponseOk<ResultsPage<UninitializedSled>>, HttpError> {
let apictx = rqctx.context();
let handler = async {
query: Query<PaginationParams<EmptyScanParams, ()>>,
) -> Result<HttpResponseOk<ResultsPage<UninitializedSled>>, HttpError> {
let pag_params = query.into_inner();
if let dropshot::WhichPage::Next(last_seen) = &pag_params.page {
return Err(Error::invalid_value(last_seen.clone(), "bad page token").into());
};
let apictx = rqctx.context();
let handler = async {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! Fixed in 06a3851.

Comment on lines 4045 to 4051
},
{
"in": "query",
"name": "sort_by",
"schema": {
"$ref": "#/components/schemas/IdSortMode"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally this should not be here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 06a3851.

Comment on lines +4072 to +4073
"x-dropshot-pagination": {
"required": []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lfgtm

@andrewjstone andrewjstone merged commit 6783a5a into main Dec 19, 2023
21 checks passed
@andrewjstone andrewjstone deleted the sled-uninit-list-fix-fake-pagination branch December 19, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants